Commit to payment_metadata in inbound payment HMAC by TheBlueMatt · Pull Request #4528 · lightningdevkit/rust-lightning

TheBlueMatt · 2026-03-31T19:23:00Z

When payment_metadata is set in a BOLT 11 invoice, users expect to
receive it back as-is in the payment onion. In order to ensure it
isn't tampered with, they presumably will add an HMAC, or worse, not
add one and forget that it can be tampered with.

Instead, here we include it in the HMAC computation for the payment
secret. This ensures that the sender must relay the correct
metadata for the payment to be accepted by the receiver, binding
the metadata to the payment cryptographically.

The metadata is only included in the HMAC when present, so existing
payments without metadata continue to verify correctly. However,
this does break receiving payments with metadata today. On an
upgrade this seems acceptable to me given we have seen almost no
use of payment metadata in practice.

ldk-reviews-bot · 2026-03-31T19:23:10Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

ldk-claude-review-bot · 2026-03-31T19:44:42Z

All call sites are accounted for and updated. I've thoroughly reviewed the entire PR diff. My prior review comments cover the substantive issues. Let me verify the changelog is fixed now.

The changelog has "committed" (not "comitted"), so that prior comment is resolved.

I have no new issues to report beyond what was already covered in my prior review pass. The HMAC computation is consistent across all code paths, all API callers are updated, and the test coverage is reasonable (though as noted previously, get_payment_preimage with non-None metadata lacks a dedicated test).

No new issues found beyond what was already flagged in prior review passes.

Prior comments status:

pending_changelog/matt-commit-to-metadata.txt:2 — Resolved (typo fixed).
lightning/src/ln/inbound_payment.rs:182 — Retracted (length prefix properly distinguishes None from Some(&[])).
lightning/src/ln/channelmanager.rs:14983 — Still applicable: create_inbound_payment docs don't mention the new payment_metadata parameter.
lightning/src/ln/channelmanager.rs:15067 — Still applicable: get_payment_preimage docs don't explain that the same payment_metadata used at creation time must be passed.

Cross-cutting concern (still applicable from prior review):

No test exercises get_payment_preimage with non-None metadata. If a user creates a payment with metadata via create_inbound_payment(..., Some(&metadata)) and later calls get_payment_preimage(hash, secret, None), it will silently return an APIMisuseError because the derived preimage won't match. A test would catch regressions and serve as documentation of this invariant.

ldk-reviews-bot · 2026-04-02T19:33:29Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

LGTM aside from CI and one or two of Claude's doc nits

valentinewallace · 2026-04-03T15:07:52Z

+	///
+	/// Note that because it is exposed to the sender in the invoice you should consider encrypting
+	/// it. It is committed to, however, so cannot be modified by the sender.
+	pub payment_metadata: Option<Vec<u8>>,


nit: this could've been a separate commit

elnosh · 2026-04-23T19:33:29Z

+		let raw_invoice = if let Some(payment_metadata) = payment_metadata {
+			invoice.payment_metadata(payment_metadata).build_raw()


check length of payment_metadata and return error if greater than max allowed length of field in invoice?

Hmm, lightning-invoice isn't aware of a limit - if there is one we should enforce it everywhere which seems like an orthogonal PR.

shouldn't it be aware of the protocol limit here? https://github.com/TheBlueMatt/rust-lightning/blob/4bf195c5edae74699e9d7a9f598fa99a04679c29/lightning-invoice/src/ser.rs#L427

so passing metadata in the Bolt11InvoiceParameters above this would cause ldk to panic.

#[test] fn test_create_invoice_payment_metadata_too_long() { let chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); let description = Bolt11InvoiceDescription::Direct( Description::new("Some description".to_string()).unwrap(), ); let invoice_params = Bolt11InvoiceParameters { amount_msats: Some(10_000), description, payment_metadata: Some(vec![0; 640]), ..Default::default() }; let _ = nodes[1].node.create_bolt11_invoice(invoice_params); }

tnull · 2026-05-06T12:33:24Z

@TheBlueMatt any chance to get this into 0.3 still? We'd need it to make lightningdevkit/ldk-node#899 safe, which we want to do given we're now doing #4584 ^^

And, given this PR breaks backwards compat. for payment metadata users, we'll probably want to have the breakage happen before we start using payment metadata in LDK Node (i.e. lightningdevkit/ldk-node#899).

Feel free to object, but for that reason I'm adding this to the 0.3 milestone.

TheBlueMatt · 2026-05-06T19:30:45Z

Rebased and ~addressed feedback.

codecov · 2026-05-06T21:16:26Z

Codecov Report

❌ Patch coverage is 94.26230% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.12%. Comparing base (8882edd) to head (44828f7).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/inbound_payment.rs	95.23%	4 Missing ⚠️
lightning/src/ln/channelmanager.rs	87.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4528   +/-   ##
=======================================
  Coverage   86.12%   86.12%           
=======================================
  Files         157      157           
  Lines      108824   108922   +98     
  Branches   108824   108922   +98     
=======================================
+ Hits        93721    93808   +87     
- Misses      12487    12496    +9     
- Partials     2616     2618    +2

Flag	Coverage Δ
tests	`86.12% <94.26%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

Changes look good, feel free to squash.

tnull · 2026-05-08T10:25:31Z

+const INFO_KEY_LEN: usize = 32;
 const AMT_MSAT_LEN: usize = 8;
-// Used to shift the payment type bits to take up the top 3 bits of the metadata bytes, or to
+// Used to shift the payment type bits to take up the top 3 bits of the info bytes, or to


It seems @jkczyz might have an opinion here. IIRC he's of the opinion 'everything is information, so naming something "info" doesn't add anything'.

Yea, I don't love calling it just "info", I'm definitely open to better names. "metadata" is obviously out as ambiguous, but I don't have strong feelings at all.

tnull · 2026-05-08T11:41:19Z

+	/// onion by the sender, available as [`RecipientOnionFields::payment_metadata`] via
+	/// [`Event::PaymentClaimable::onion_fields`].
+	///
+	/// Note that because it is exposed to the sender in the invoice you should consider encrypting


Utilities for encryption will be part of lightningdevkit/ldk-node#899, but I do wonder if we should maybe offer something similar upstream?

I mean without needing authentication its just "ChaCha it"? Not sure we need a utility to call chacha.

TheBlueMatt · 2026-05-11T00:08:27Z

Rebased and squashed.

`payment_metadata` is a separate concept at the BOLT 11 layer (similar to payment secret, but arbitrary-sized) and at the BOLT 12 layer, so referring to payment information as "payment metadata" is confusing. Instead, use simply "payment info".

When payment_metadata is set in a BOLT 11 invoice, users expect to receive it back as-is in the payment onion. In order to ensure it isn't tampered with, they presumably will add an HMAC, or worse, not add one and forget that it can be tampered with. Instead, here we include it in the HMAC computation for the payment secret. This ensures that the sender must relay the correct metadata for the payment to be accepted by the receiver, binding the metadata to the payment cryptographically. The metadata is only included in the HMAC when present, so existing payments without metadata continue to verify correctly. However, this does break receiving payments with metadata today. On an upgrade this seems acceptable to me given we have seen almost no use of payment metadata in practice. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Now that we commit to payment metadata fields and require them implicitly as a part of payments, we should match that in `lightning-invoice` - instead marking them as required by default.

ldk-reviews-bot requested a review from valentinewallace March 31, 2026 19:33

ldk-claude-review-bot reviewed Mar 31, 2026

View reviewed changes

Comment thread pending_changelog/matt-commit-to-metadata.txt Outdated

ldk-claude-review-bot reviewed Mar 31, 2026

View reviewed changes

Comment thread lightning/src/ln/inbound_payment.rs

ldk-claude-review-bot reviewed Mar 31, 2026

View reviewed changes

Comment thread lightning/src/ln/channelmanager.rs

ldk-claude-review-bot reviewed Mar 31, 2026

View reviewed changes

Comment thread lightning/src/ln/channelmanager.rs

valentinewallace reviewed Apr 3, 2026

View reviewed changes

elnosh reviewed Apr 23, 2026

View reviewed changes

tnull self-requested a review May 6, 2026 12:33

tnull mentioned this pull request May 6, 2026

Move BOLT11 JIT params to payment metadata lightningdevkit/ldk-node#899

Open

tnull added this to the 0.3 milestone May 6, 2026

TheBlueMatt force-pushed the 2026-03-commit-to-metadata branch 4 times, most recently from cff21e1 to 971de9d Compare May 6, 2026 19:30

TheBlueMatt force-pushed the 2026-03-commit-to-metadata branch from 971de9d to 4bf195c Compare May 6, 2026 19:33

tnull reviewed May 8, 2026

View reviewed changes

TheBlueMatt force-pushed the 2026-03-commit-to-metadata branch from 4bf195c to ee26f5c Compare May 11, 2026 00:08

TheBlueMatt and others added 3 commits May 11, 2026 00:13

Default to requiring payment_metadata when building BOLT 11s

44828f7

Now that we commit to payment metadata fields and require them implicitly as a part of payments, we should match that in `lightning-invoice` - instead marking them as required by default.

TheBlueMatt force-pushed the 2026-03-commit-to-metadata branch from ee26f5c to 44828f7 Compare May 11, 2026 00:13

		let raw_invoice = if let Some(payment_metadata) = payment_metadata {
		invoice.payment_metadata(payment_metadata).build_raw()

Conversation

TheBlueMatt commented Mar 31, 2026

Uh oh!

ldk-reviews-bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-claude-review-bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 2, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

elnosh Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 6, 2026

Choose a reason for hiding this comment

Uh oh!

elnosh May 6, 2026

Choose a reason for hiding this comment

Uh oh!

tnull commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented May 6, 2026

Uh oh!

codecov Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull May 8, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 10, 2026

Choose a reason for hiding this comment

Uh oh!

tnull May 8, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 10, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ldk-reviews-bot commented Mar 31, 2026 •

edited

Loading

ldk-claude-review-bot commented Mar 31, 2026 •

edited

Loading

elnosh Apr 23, 2026 •

edited

Loading

tnull commented May 6, 2026 •

edited

Loading

codecov Bot commented May 6, 2026 •

edited

Loading